Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify storage options #211

Merged
merged 8 commits into from
Aug 2, 2019
Merged

Unify storage options #211

merged 8 commits into from
Aug 2, 2019

Conversation

difernandez
Copy link
Contributor

Until now, the inclusion of active_storage or paperclip were two different asks. That meant that, theoretically, the user could enable both, which would result in unexpected behaviour.
This PR unifies both under de storage key and the file_storage recipe

Changes

  • Use Ask.list to get value of :storage. It can be active_storage, paperclip or none
  • Common setup for both storages was extracted to a method
  • Change cli_options and the application template accordingly
  • Moves active_storage tests and added some for paperclip
  • Removed AWS_REGION env var setting when selecting active_storage, it was already being set in env.development template

@difernandez difernandez requested a review from rjherrera July 12, 2019 23:38
Copy link
Member

@rjherrera rjherrera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@difernandez difernandez requested a review from blackjid July 15, 2019 13:10
type: :flag,
name: "storage",
desc: "Decides which file storage to use. Available: active_storage, paperclip, none",
default_test_value: "active_storage"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no dejarias active_storage por defecto?? creo que es el que debieramos empujar mas a usar. Sobre todo que paperclip esta deprecado...

Copy link
Member

@blackjid blackjid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

De hecho, creo que me vas a matar..... no seria razonbale sacar paperclip en verdad? Paperclip esta deprecado, no esta en desarrollo. Probablemente el mismo thoughbot usa activestorage ahora.

Sabes de algun proyecto nuevo en que hayamos decidido usar paperclip en vez de activestorage? hay cosas que todavia activestorage no puede hacer?

@blackjid
Copy link
Member

Otra cosa seria buena agregar como dependencia la libreria que usemos para manejo de imagenes.. o no? Creo que es un caso muy comun. Para eso hay que agregar en el gemfile la gema image-processing. En el pasado hemos agregado imagemagick en en aptfile para que el buildpack lo agregue, pero tal vez es mejor usar https://github.com/minimagick/minimagick, que es una gema que agrega la libreria.... creo que es mas ordenado y mantenemos control sobre la version usando bundler no mas...

Paso adicional, seria evaluar https://github.com/libvips/ruby-vips, que es otra libreria de manejo de imagenes que claim que usar mucho menos memoria y es mucho mas rapido que usar imagemagick. Activestorage tiene soporte para esa libreria tambien.

https://edgeapi.rubyonrails.org/classes/ActiveStorage/Variant.html
https://edgeguides.rubyonrails.org/active_storage_overview.html#transforming-images

@difernandez
Copy link
Contributor Author

@blackjid se que se usa en varios proyectos, al menos ksec y pic-parks lo usan, pero proyectos nuevos no que yo sepa, quizás sería mejor eliminarlo (o al menos agregar un mensaje a la opción que indique que está deprecado). Había pensado en eso pero recuerdo que nos habíamos topado con problemas que si no me equivoco tenían que ver con ActiveStorage, como lo de las imagenes que a veces no se ven en refreshments (creo que tenía que ver con este issue). Por eso había pensado en mantenerlo, pero abierto a sacarlo si se cree que eso es mejor.

Con respecto al procesador de imagenes, tengo unas dudas:

  • Hasta ahora potassium no se mete con nada de eso, ni con las gemas ni el aptfile no?
  • Por lo que entiendo image-processing ya tiene a mini-magick como dependencia, pero esta última requiere igual que image-magick esté instalado, entonces el aptfile igual sería necesario o me equivoco?
  • Esta feature de dejar instalado y funcionando un procesador de imagenes te lo imaginas como opcional (otro ask después de elegir storage) o como comportamiento por defecto? Pregunto porque no se bien que tan común es este caso de uso, dado que se necesita un storage

@blackjid
Copy link
Member

  • Hasta ahora potassium no se mete con nada de eso, ni con las gemas ni el aptfile no?

No, no lo hace, pero no se si es por decision de diseño o por que no hemos tenido la necesidad. Es algo que es bastante heroku depentiende, al menos la instalacion de apt.

Toda la razon

  • Esta feature de dejar instalado y funcionando un procesador de imagenes te lo imaginas como opcional (otro ask después de elegir storage) o como comportamiento por defecto? Pregunto porque no se bien que tan común es este caso de uso, dado que se necesita un storage

A mi me tinca que quede por defecto, pero en verdad no estoy seguro que tanto se usa.

Me tinca la idea de dejar paperclip con un [DEPRECADET] para desinsentivar su uso. En un siguiente release podemos matarlo.

Te tinca tiempo abrir un issue con el tema de imagemagic, vips, y el apt y tirarlo a discucion??

@difernandez
Copy link
Contributor Author

Ya, agrego unos commits para dejar active_storage como default_value en las opciones y para agregar el mensaje de deprecado y redacto un issue para que discutamos lo del procesador de imagen

@difernandez difernandez requested a review from blackjid July 26, 2019 14:08
@difernandez
Copy link
Contributor Author

@blackjid agregué el mensaje de deprecado y el default. Me queda una duda sí, el default sirve de algo cuando la pregunta es tipo flag? Me da la impresión de que solo serviría cuando es switch, porque ahí está la posibilidad de que el usuario presione Enter no más sin decir si/no

@difernandez difernandez force-pushed the unify-storage-options branch from 09d046f to d3a3f17 Compare July 26, 2019 14:29
@blackjid
Copy link
Member

Buena! No estoy seguro, puede ser que el default tenga más que ver con cuál es la primera que está seleccionada. Así uno puede apretar entre y se selecciona y funciona igual que para el switch

@difernandez
Copy link
Contributor Author

@blackjid ya entonces lo dejo así por si acaso

@difernandez difernandez merged commit 8e2a9db into master Aug 2, 2019
@difernandez difernandez deleted the unify-storage-options branch August 2, 2019 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants